Dialog: dynamically switch footer button layout based on available height#7683
Dialog: dynamically switch footer button layout based on available height#7683liuliu-dev wants to merge 4 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 2c08367 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
github-actions
bot
added
the
integration-tests: recommended
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/16505 |
There was a problem hiding this comment.
Pull request overview
This PR updates Dialog footer button layout behavior to avoid 2D scrolling in very small viewports by dynamically choosing between wrapped buttons and horizontal scrolling based on available vertical space.
Changes:
- Add runtime measurement logic (via
useResizeObserver) to decide whether footer buttons should wrap or horizontally scroll. - Replace the previous fixed
@media (max-height: 325px)footer rule with adata-footer-button-layout-driven CSS selector. - Add a patch changeset entry for the
@primer/reactrelease.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/react/src/Dialog/Dialog.tsx | Adds measurement + state to set data-footer-button-layout based on computed available body height. |
| packages/react/src/Dialog/Dialog.module.css | Swaps the fixed viewport-height media query for an attribute-driven “scroll” footer layout rule. |
| .changeset/neat-moose-dress.md | Declares a patch release note for the Dialog footer behavior change. |
| const viewportHeight = backdropRef.current?.clientHeight ?? window.innerHeight | ||
| const positionRegular = dialogElement.getAttribute('data-position-regular') | ||
| const positionNarrow = dialogElement.getAttribute('data-position-narrow') | ||
| // fullscreen/left/right fill the full viewport; otherwise match CSS max-height gutter. | ||
| const gutter = viewportHeight <= 280 ? 12 : 64 | ||
| const dialogMaxHeight = | ||
| positionNarrow === 'fullscreen' || positionRegular === 'left' || positionRegular === 'right' | ||
| ? viewportHeight | ||
| : Math.max(0, viewportHeight - gutter) | ||
|
|
There was a problem hiding this comment.
The gutter/dialogMaxHeight calculation re-implements CSS max-height logic, but it doesn’t match the stylesheet: 12px gutter is only applied under @media (max-width: 767px) { @media (max-height: 280px) { ... } } in Dialog.module.css, while this code applies 12 whenever viewportHeight <= 280 regardless of viewport width. This can cause the footer layout decision to diverge from the actual dialog max-height. Consider deriving the max height from computed styles (e.g. getComputedStyle(dialogElement).maxHeight) or mirroring the same width breakpoint check before switching to the 12px gutter.
|
|
||
| setFooterButtonLayout(visibleBodyHeightWithWrap >= MIN_BODY_HEIGHT ? 'wrap' : 'scroll') | ||
| }, [hasFooter]) | ||
|
|
There was a problem hiding this comment.
updateFooterButtonLayout is only triggered via useResizeObserver(..., backdropRef), so the layout may become stale when footer/header content changes without a viewport/backdrop resize (e.g. button label changes, async loading states, toggling footerButtons). Additionally, in the useResizeObserver fallback path (no ResizeObserver), the callback isn’t invoked on mount—only on window.resize—so data-footer-button-layout may never be set correctly initially. Consider calling updateFooterButtonLayout() in an effect when relevant inputs change (and/or observing the dialog/footer element too) so the layout decision is updated even without a resize event.
| useEffect(() => { | |
| updateFooterButtonLayout() | |
| }, [updateFooterButtonLayout]) |
| function measureWrappedFooterHeight(footerElement: HTMLElement) { | ||
| const measurementContainer = document.createElement('div') | ||
| const measuredFooter = footerElement.cloneNode(true) as HTMLElement | ||
|
|
||
| Object.assign(measurementContainer.style, { | ||
| position: 'fixed', | ||
| top: '0', | ||
| left: '-99999px', | ||
| visibility: 'hidden', | ||
| pointerEvents: 'none', | ||
| contain: 'layout style size', | ||
| }) | ||
|
|
||
| measuredFooter.style.width = `${footerElement.getBoundingClientRect().width}px` | ||
|
|
||
| Object.assign(measuredFooter.style, { | ||
| flexWrap: 'wrap', | ||
| overflowX: '', | ||
| overflowY: '', | ||
| justifyContent: '', | ||
| }) | ||
|
|
||
| measurementContainer.appendChild(measuredFooter) | ||
| document.body.appendChild(measurementContainer) | ||
|
|
||
| const measuredHeight = measuredFooter.offsetHeight | ||
| measurementContainer.remove() | ||
|
|
There was a problem hiding this comment.
measureWrappedFooterHeight clones the entire footer, inserts it into document.body, forces layout, then removes it on every resize observer callback. This can be relatively expensive during viewport changes (especially on mobile where viewport height can change frequently). Consider caching/reusing a single measurement container and only re-measuring when width/footer content changes, to reduce DOM churn and forced reflow.
| data-has-footer={hasFooter ? '' : undefined} | ||
| data-footer-button-layout={hasFooter ? footerButtonLayout : undefined} |
There was a problem hiding this comment.
This PR introduces a new data-footer-button-layout attribute that changes footer behavior based on measured heights, but Dialog.test.tsx doesn’t currently cover this new behavior. Adding a unit test that mocks ResizeObserver and element measurements (e.g. offsetHeight / getBoundingClientRect) would help prevent regressions for the wrap-vs-scroll switching logic.
Fixes an accessibility issue where Dialog footer buttons caused 2D scrolling at small viewport sizes (e.g. 320×256px).
Previously, the footer switched to horizontal scroll via a fixed
@media (max-height: 325px)rule. Now the footer wraps (default) when there's enough room for body content (≥56px), and it scrolls horizontally when the viewport is too short to fit both wrapped footer and minimum body content.Tested the fix in https://github.com/github/primer-docs/pull/977
Rollout strategy
Testing & Reviewing
Merge checklist